Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

provider/azurerm: Fix VHD deletion when VM and Storage account are in separate resource groups #9631

Merged
merged 7 commits into from
Oct 27, 2016
Merged

provider/azurerm: Fix VHD deletion when VM and Storage account are in separate resource groups #9631

merged 7 commits into from
Oct 27, 2016

Conversation

carinadigital
Copy link
Contributor

@carinadigital carinadigital commented Oct 26, 2016 •
edited
Loading

delete_os_disk_on_termination and delete_data_disk_on_termination don't always remove the VHD disk blob when deleting a Virtual Machine.
This is because it assumes the storage account resource group is the same as the virtual machine's resource group.

Unfortunately I can't find any information on the virtual machine that stores the storage account resource group and so I've had to go back the API. The only method I've found is to list the resources here https://godoc.org/github.com/Azure/azure-sdk-for-go/arm/resources/resources#Client.List

Other suggestions are welcome.

pmcatominey
pmcatominey suggested changes Oct 26, 2016
View reviewed changes
Copy link
Contributor

@pmcatominey pmcatominey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice spot on this case!

Could you add a test for this scenario?

Cheers

builtin/providers/azurerm/config.go
@@ -314,6 +315,12 @@ func (c *Config) getArmClient() (*ArmClient, error) {
tc.Sender = autorest.CreateSender(withRequestLogging())
client.tagsClient = tc

rf := resources.NewClient(c.SubscriptionID)
setUserAgent(&tc.Client)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tc.Client => rf.Client

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good spot 👍

@carinadigital
Copy link
Contributor Author

I'm currently working on improving the current VHD opt in test case.

@carinadigital
Copy link
Contributor Author

I will run a full set of VM tests tomorrow. If they pass, I'll make this ready for review.

@stack72
Copy link
Contributor

stack72 commented Oct 26, 2016

Thanks @carinadigital :)

@carinadigital
Copy link
Contributor Author

carinadigital commented Oct 27, 2016 •
edited
Loading

Original code failing with new test at c1d41e2 - Improve VHD deletion test when storage account is in different resource group

root@ad8543247232:/go/src/github.com/hashicorp/terraform# make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAzureRMVirtualMachine_deleteVHDOptIn'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/10/27 08:43:30 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMVirtualMachine_deleteVHDOptIn -timeout 120m
=== RUN   TestAccAzureRMVirtualMachine_deleteVHDOptIn
--- FAIL: TestAccAzureRMVirtualMachine_deleteVHDOptIn (697.60s)
        testing.go:265: Step 1 error: Check failed: Check 1/2 error: Disk VHD Blob still exists
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/azurerm        697.608s
Makefile:47: recipe for target 'testacc' failed
make: *** [testacc] Error 1

Tests after new commits added.

TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAzureRMVirtualMachine -timeout 120m
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux (541.38s)
=== RUN   TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears
--- PASS: TestAccAzureRMVirtualMachineScaleSet_basicLinux_disappears (692.03s)
=== RUN   TestAccAzureRMVirtualMachine_basicLinuxMachine
--- PASS: TestAccAzureRMVirtualMachine_basicLinuxMachine (558.80s)
=== RUN   TestAccAzureRMVirtualMachine_basicLinuxMachine_disappears
--- PASS: TestAccAzureRMVirtualMachine_basicLinuxMachine_disappears (439.40s)
=== RUN   TestAccAzureRMVirtualMachine_withDataDisk
--- PASS: TestAccAzureRMVirtualMachine_withDataDisk (682.78s)
=== RUN   TestAccAzureRMVirtualMachine_tags
--- PASS: TestAccAzureRMVirtualMachine_tags (831.82s)
=== RUN   TestAccAzureRMVirtualMachine_updateMachineSize
--- PASS: TestAccAzureRMVirtualMachine_updateMachineSize (836.10s)
=== RUN   TestAccAzureRMVirtualMachine_basicWindowsMachine
--- PASS: TestAccAzureRMVirtualMachine_basicWindowsMachine (930.16s)
=== RUN   TestAccAzureRMVirtualMachine_windowsUnattendedConfig
--- PASS: TestAccAzureRMVirtualMachine_windowsUnattendedConfig (928.82s)
=== RUN   TestAccAzureRMVirtualMachine_diagnosticsProfile
--- PASS: TestAccAzureRMVirtualMachine_diagnosticsProfile (820.41s)
=== RUN   TestAccAzureRMVirtualMachine_winRMConfig
--- PASS: TestAccAzureRMVirtualMachine_winRMConfig (928.24s)
=== RUN   TestAccAzureRMVirtualMachine_deleteVHDOptOut
--- PASS: TestAccAzureRMVirtualMachine_deleteVHDOptOut (632.53s)
=== RUN   TestAccAzureRMVirtualMachine_deleteVHDOptIn
--- PASS: TestAccAzureRMVirtualMachine_deleteVHDOptIn (601.82s)
=== RUN   TestAccAzureRMVirtualMachine_ChangeComputerName
--- PASS: TestAccAzureRMVirtualMachine_ChangeComputerName (984.77s)
=== RUN   TestAccAzureRMVirtualMachine_ChangeAvailbilitySet
--- PASS: TestAccAzureRMVirtualMachine_ChangeAvailbilitySet (973.31s)

@stack72
Copy link
Contributor

stack72 commented Oct 27, 2016

Hey @carinadigital

Is there any way that we can test this within the nightly acceptance tests?

P.

@stack72
Copy link
Contributor

stack72 commented Oct 27, 2016

ah wait, I just noticed that you have already taken care of it within a test :)

@carinadigital
Copy link
Contributor Author

I've improved the TestAccAzureRMVirtualMachine_deleteVHDOptIn() test. I didn't think we needed a deletion test for both disk in same resource group and separate resource group.

@carinadigital
Copy link
Contributor Author

Full test results passing above. On that note, a reminder to myself to proposing increasing acceptance test timeout. 120 minutes is far too short for the VM tests.

Ready for review.

@carinadigital carinadigital changed the title [WIP] provider/azurerm: Fix VHD deletion when VM and Storage account are in separate resource groups provider/azurerm: Fix VHD deletion when VM and Storage account are in separate resource groups Oct 27, 2016
pmcatominey
pmcatominey suggested changes Oct 27, 2016
View reviewed changes
builtin/providers/azurerm/resource_arm_virtual_machine.go
// Storage Account ID is in the form
// /subscriptions/[SUBSCRIPTION_ID]/resourceGroups/[RESOURCE_GROUP]/providers/Microsoft.Storage/storageAccounts/[STORAGE_ACCOUNT]
idSplit := strings.Split(strings.TrimPrefix(*results[0].ID, "/"), "/")
storageAccountResourceGroupName := idSplit[3]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using parseAzureResourceID would be cleaner here, would allow you to catch any errors too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I didn't know that existed. Fixing.

@stack72
Copy link
Contributor

stack72 commented Oct 27, 2016

Thanks for the work here @carinadigital :) This LGTM!

P.

@stack72 stack72 merged commit 59d0362 into hashicorp:master Oct 27, 2016
antonbabenko pushed a commit to antonbabenko/terraform that referenced this pull request Oct 27, 2016
@carinadigital @antonbabenko
provider/azurerm: Fix VHD deletion when VM and Storage account are in…
d624944 
… separate resource groups (hashicorp#9631)

* Improve messaging when storage account isn't found.

* Add client for finding resources when you don't know it's resource group.

* Add function to find Storage Account resource group name.

* Use the storage account resource group, not the virtual machine's resource group when deleting VHDs.

* Add description of storage account ID for clarity.

* Improve VHD deletion test when storage account is in different resource group.

* Use common function for ID parsing of storage account.
mathieuherbert pushed a commit to mathieuherbert/terraform that referenced this pull request Oct 30, 2016
@carinadigital
provider/azurerm: Fix VHD deletion when VM and Storage account are in…
868d93f 
… separate resource groups (hashicorp#9631)

* Improve messaging when storage account isn't found.

* Add client for finding resources when you don't know it's resource group.

* Add function to find Storage Account resource group name.

* Use the storage account resource group, not the virtual machine's resource group when deleting VHDs.

* Add description of storage account ID for clarity.

* Improve VHD deletion test when storage account is in different resource group.

* Use common function for ID parsing of storage account.
gusmat pushed a commit to gusmat/terraform that referenced this pull request Dec 6, 2016
@carinadigital
provider/azurerm: Fix VHD deletion when VM and Storage account are in…
b30ef28 
… separate resource groups (hashicorp#9631)

* Improve messaging when storage account isn't found.

* Add client for finding resources when you don't know it's resource group.

* Add function to find Storage Account resource group name.

* Use the storage account resource group, not the virtual machine's resource group when deleting VHDs.

* Add description of storage account ID for clarity.

* Improve VHD deletion test when storage account is in different resource group.

* Use common function for ID parsing of storage account.
@ghost
Copy link

ghost commented Apr 21, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pmcatominey pmcatominey pmcatominey requested changes

Assignees
No one assigned
Labels
bug provider/azurerm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@carinadigital @stack72 @pmcatominey